TestJumpToDateEndpoint: Add test for seamlessly paginating from /context start token#853
Conversation
… used to paginate forwards with."
| }) | ||
|
|
||
| t.Run("can paginate after getting remote event from timestamp to event endpoint", func(t *testing.T) { | ||
| t.Run("can paginate backwards after getting remote event from timestamp to event endpoint (start)", func(t *testing.T) { |
There was a problem hiding this comment.
This test will fail until element-hq/synapse#19611 is merged
| }) | ||
|
|
||
| t.Run("can paginate after getting remote event from timestamp to event endpoint", func(t *testing.T) { | ||
| t.Run("can paginate backwards after getting remote event from timestamp to event endpoint (start)", func(t *testing.T) { |
There was a problem hiding this comment.
Updated to have a test for paginating from the start and end tokens from /context after getting an event from /timestamp_to_event.
Previously, we only had a test for end
| // Hit `/messages` until `eventA` has been backfilled and replicated across | ||
| // workers (the worker persisting events isn't necessarily the same as the worker | ||
| // serving `/messages`) | ||
| fetchUntilMessagesResponseHas(t, remoteCharlie, roomID, func(ev gjson.Result) bool { | ||
| return ev.Get("event_id").Str == eventA.EventID | ||
| t.Run("can paginate backwards after getting remote event from timestamp to event endpoint (end)", func(t *testing.T) { | ||
| t.Parallel() | ||
| roomID, eventA, eventB := createTestRoom(t, alice) | ||
| remoteCharlie.MustJoinRoom(t, roomID, []spec.ServerName{ | ||
| deployment.GetFullyQualifiedHomeserverName(t, "hs1"), | ||
| }) |
There was a problem hiding this comment.
While the idea of the comment above fetchUntilMessagesResponseHas(...) is true, we're retrying the wrong endpoint. When you use /timstamp_to_event, it should backfill the event in the cases of asking the federation. Which means we should be retrying the /context request until we see the event (not /messages)
This functionality was maintained via the client.WithRetryUntil(...) on the /context request
| } | ||
| } | ||
|
|
||
| func fetchUntilMessagesResponseHas(t *testing.T, c *client.CSAPI, roomID string, check func(gjson.Result) bool) { |
There was a problem hiding this comment.
Remove fetchUntilMessagesResponseHas as it's now unused
…om-start Conflicts: tests/room_timestamp_to_event_test.go
There was a problem hiding this comment.
LGTM, assuming /context -> /messages makes sense, and CI eventually passes.
Thank you for the detailed comments - they're especially helpful with such a complex topic.
The juicy details and explanation are in the diff itself. Split out from #18873 in order to fix paginating from [MSC3871](matrix-org/matrix-spec-proposals#3871) gap tokens actually backfilling history. To be clear, this is a good change to make outside of the [MSC3871](matrix-org/matrix-spec-proposals#3871) use case. For example (as the new Complement test shows), fixes a problem where if you try to paginate `/messages` from tokens returned by `/context`, we could fail to backfill anything new and hide away history. Also fixes matrix-org/complement#853
|
(this was closed automatically by GitHub because the description in element-hq/synapse#19611 said this Re-opening as we want to merge this ⏩ |
MadLittleMods
deleted the
madlittlemods/timestamp-to-event-paginate-from-start
branch
|
Thanks for the review @anoadragon453 🦑 |
2 participants
TestJumpToDateEndpoint: Add test for seamlessly paginating from/contextstarttokenFollow-up to #852
Dev notes
Todo
TestJumpToDateEndpoint: Clarify why we use/contextendfor paginating backwards despite the spec saying "A token that can be used to paginate forwards with." #852 to mergePull Request Checklist